docs(pr-review): tighten weak language and add trigger phrase - #753
Conversation
PR Summary by QodoPR review skill docs: add trigger phrase and tighten verification/footers language
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
🤖 Finished Review · ✅ Success · Started 5:01 PM UTC · Completed 5:14 PM UTC Commit: |
Code Review by Qodo
1. Protected skills/ file modified
|
ReviewFindingsMedium
Previous runReviewFindingsMedium
Previous run (2)ReviewFindingsMedium
Previous run (3)ReviewFindingsMedium
Previous run (4)ReviewFindingsMedium
Previous run (5)ReviewFindingsMedium
Previous run (6)ReviewFindingsMedium
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
Previous run (9)ReviewFindingsMedium
Low
Labels: PR modifies the pr-review skill definition (skills/pr-review/SKILL.md), a documentation-only change to the review agent orchestrator skill |
waynesun09
left a comment
There was a problem hiding this comment.
MEDIUM: Parallel 'Insufficient context' bullet still hedges with "note that" instead of "state that"
File: skills/pr-review/SKILL.md (line ~1039, outside this PR's diff hunks, so not addressable as an inline comment)
The PR converts exactly three "note that" -> "state that" instances (the omitted-files line, the PR-head-verification line, and the case-2 "Sufficient context" protected-path bullet), but leaves the structurally identical case-1 "Insufficient context" bullet two lines above untouched. It still reads:
The description MUST list the affected protected files and note that the PR lacks justification for modifying governance or infrastructure files.
while the case-2 sibling now reads "...and state that human approval is always required...". Both bullets share the same "MUST list ... and {note|state} that ..." template for the two branches of the protected-path check, so leaving one on the old hedge word reintroduces the exact weak-language inconsistency this PR is meant to eliminate.
Suggestion: Change the case-1 bullet's "...and note that the PR lacks justification..." to "...and state that the PR lacks justification..." to match the sibling bullet.
|
/fs-fix address @waynesun09's findings. |
|
🤖 Finished Fix · ✅ Success · Started 8:44 PM UTC · Completed 8:49 PM UTC Commit: |
🔧 Fix agent — iteration 1 (human-triggered)Addressed waynesun09's finding: replaced the remaining 'note that' hedge in the protected-path Insufficient context bullet with 'state that' to match its sibling bullet. One pre-existing test failure (prerequisites-creates-allowed-issue) is unrelated to this change. Fixed (1):
Tests: passedNext steps:
|
Change the 'Insufficient context' bullet's 'and note that' to 'and state that' to match the sibling 'Sufficient context' bullet, closing the weak-language inconsistency flagged by @waynesun09. Addresses review feedback on #753
|
🤖 Finished Review · ✅ Success · Started 8:50 PM UTC · Completed 9:02 PM UTC Commit: |
bc717f2 to
2d76cd1
Compare
|
🤖 Finished Review · ✅ Success · Started 9:39 PM UTC · Completed 9:53 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Reviewed via 4-agent squad (claude-coder, claude-researcher, grok-review-agent, cursor-code-review) + verification pass at head 2d76cd1 — no CRITICAL/HIGH/MEDIUM findings survived verification.
|
/fs-fix fix the "New trigger phrase appended" problem. |
|
🤖 Finished Fix · ✅ Success · Started 10:40 PM UTC · Completed 10:49 PM UTC Commit: |
🔧 Fix agent — iteration 2 (human-triggered)Addressed 2 findings: (1) rewrote the redundant trigger phrase that repeated 'structured review result' from the preceding description sentence, and (2) fixed the docs-review link formatting to match the security-triage link pattern. Pre-existing test failures in post-retro-test.sh (10 tests) are unrelated to this change. Fixed (2):
Tests: passedNext steps:
|
Rewrites the "Use when" trigger phrase to avoid repeating "structured review result" from the preceding sentence. Also removes double quotes around the docs-review markdown link to match the security-triage link pattern at line 438. Addresses review feedback on #753
|
🤖 Finished Review · ✅ Success · Started 10:50 PM UTC · Completed 11:01 PM UTC Commit: |
|
/fs-fix move the "use when" trigger language to the beginning of the description |
|
🤖 Finished Fix · ✅ Success · Started 11:25 PM UTC · Completed 11:29 PM UTC Commit: |
|
🤖 Finished Review · ✅ Success · Started 6:42 PM UTC · Completed 6:52 PM UTC Commit: |
Ran skillsaw (evaluating it per #188) against the repo's skills. Adds a "Use when..." trigger phrase to the description, replaces three "note that" hedges with direct "state"/"must state" instructions, converts two bare file paths into markdown links, and makes the action-hints footer condition explicit (the post-review pipeline only appends it when the outcome is `request-changes`). Related to #188 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Change the 'Insufficient context' bullet's 'and note that' to 'and state that' to match the sibling 'Sufficient context' bullet, closing the weak-language inconsistency flagged by @waynesun09. Addresses review feedback on #753
Rewrites the "Use when" trigger phrase to avoid repeating "structured review result" from the preceding sentence. Also removes double quotes around the docs-review markdown link to match the security-triage link pattern at line 438. Addresses review feedback on #753
Moves the "Use when" trigger sentence from the end of the YAML frontmatter description to the beginning, per human instruction. Addresses review feedback on #753
8263546 to
a994da0
Compare
|
🤖 Finished Review · ✅ Success · Started 6:56 PM UTC · Completed 7:06 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep: 2 findings below.
|
/fs-fix address @waynesun09's two open findings. |
|
🤖 Finished Fix · ✅ Success · Started 8:20 PM UTC · Completed 8:25 PM UTC Commit: |
…tion Add "you must" to the spawn-prompt template's omitted-file instruction (line 727) so sub-agents receive the same RFC-2119 MUST strength as the orchestrator-facing guidance (line 203). Make the action-hints footer condition explicit: it fires only for the `request-changes` action, not for `reject`, `approve`, or `comment`. Addresses review feedback on #753
🔧 Fix agent — iteration 4 (human-triggered)Addressed both of @waynesun09's open findings: (1) aligned the spawn-prompt template's omitted-file instruction to use 'you must state' matching the orchestrator body's MUST strength, and (2) made the action-hints footer condition explicit about which actions do and don't receive it. Fixed (2):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 8:26 PM UTC · Completed 8:39 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep: 1 finding below.
|
🤖 Finished Retro · ❌ Failure · Started 5:32 PM UTC · Completed 5:33 PM UTC Commit: |
Change the 'Insufficient context' bullet's 'and note that' to 'and state that' to match the sibling 'Sufficient context' bullet, closing the weak-language inconsistency flagged by @waynesun09. Addresses review feedback on #753
Rewrites the "Use when" trigger phrase to avoid repeating "structured review result" from the preceding sentence. Also removes double quotes around the docs-review markdown link to match the security-triage link pattern at line 438. Addresses review feedback on #753
Moves the "Use when" trigger sentence from the end of the YAML frontmatter description to the beginning, per human instruction. Addresses review feedback on #753
…tion Add "you must" to the spawn-prompt template's omitted-file instruction (line 727) so sub-agents receive the same RFC-2119 MUST strength as the orchestrator-facing guidance (line 203). Make the action-hints footer condition explicit: it fires only for the `request-changes` action, not for `reject`, `approve`, or `comment`. Addresses review feedback on #753
Change the 'Insufficient context' bullet's 'and note that' to 'and state that' to match the sibling 'Sufficient context' bullet, closing the weak-language inconsistency flagged by @waynesun09. Addresses review feedback on fullsend-ai#753
Rewrites the "Use when" trigger phrase to avoid repeating "structured review result" from the preceding sentence. Also removes double quotes around the docs-review markdown link to match the security-triage link pattern at line 438. Addresses review feedback on fullsend-ai#753
Moves the "Use when" trigger sentence from the end of the YAML frontmatter description to the beginning, per human instruction. Addresses review feedback on fullsend-ai#753
…tion Add "you must" to the spawn-prompt template's omitted-file instruction (line 727) so sub-agents receive the same RFC-2119 MUST strength as the orchestrator-facing guidance (line 203). Make the action-hints footer condition explicit: it fires only for the `request-changes` action, not for `reject`, `approve`, or `comment`. Addresses review feedback on fullsend-ai#753
Ran skillsaw (evaluating it per #188) against the repo's skills.
Adds a "Use when..." trigger phrase to the description, replaces
three "note that" hedges with direct "state"/"must state" instructions,
converts two bare file paths into markdown links, and makes the
action-hints footer condition explicit (the post-review pipeline only
appends it when the outcome is
request-changes).Related to #188
Assisted-by: Claude Opus 4.6 noreply@anthropic.com
Signed-off-by: Ralph Bean rbean@redhat.com
Stack created with GitHub Stacks CLI • Give Feedback 💬